Skip to content

fix: collapse the duplicated key-slot wrap/unwrap and validate the algorithm byte (#117) - #154

Merged
Xof merged 1 commit into
mainfrom
fix/117-crypto-cleanups
Aug 5, 2026
Merged

fix: collapse the duplicated key-slot wrap/unwrap and validate the algorithm byte (#117)#154
Xof merged 1 commit into
mainfrom
fix/117-crypto-cleanups

Conversation

@Xof

@Xof Xof commented Aug 5, 2026

Copy link
Copy Markdown
Collaborator

Closes #117. Also closes #103's SUPERBLOCK-RECOVERY-3, which is the same defect as CRYPTO-4 filed twice. Stacked on #153.

CRYPTO-7 / CRYPTO-6 — four hand-maintained near-copies, in the crate's highest-consequence code

unlock's doc asserted its copy was "byte-identical" to unwrap_first_matching_slot. On the unwrap side that happened to be true — the copies differed only in how they spelled the kdf-id match, and the discriminants are 1 and 2. On the wrap side it was already false:

Path argon2 params written into an HKDF slot
wrap_into (used by add_key/rotate_key) 0/0/0 — what the format doc promises
build_create_cipher (used at create) 19456/2/1 — the OWASP defaults

So a database created with Key::Raw disagreed byte-for-byte with a second raw credential added to it later, and neither matched the documented layout. That is exactly the drift a comment claiming equivalence cannot prevent and a shared function can.

open_existing now calls header.unlock(k); build_create_cipher builds an empty header and calls wrap_into, which gained an argon2_override parameter for the create path's Options::argon2_params.

Backward compatibility — the headline risk, affirmatively closed. unlock rebuilds the AAD from the stored slot bytes and derives the KEK from the stored params, and HKDF never reads those fields, so a legacy slot carrying 19456/2/1 still unwraps. The adversarial review verified this end to end: it created a database with the old code, applied this patch, then opened it, read the payload, added a second credential, and reopened under both keys.

CRYPTO-4 / SUPERBLOCK-RECOVERY-3 — the algorithm byte was write-only

deserialize gates only on zero ("plaintext"), so any nonzero value was accepted and every page fed to PageCipher, which is hardcoded to XChaCha20-Poly1305. Because the DEK unwrap is a separate primitive that does not depend on the page algorithm, such a file opened — and then failed later as DecryptionFailed, which is_fatal() and poisons. "This build cannot read that algorithm" was reported to the user as unrecoverable data corruption.

Now rejected with EncryptionNotSupported, whose doc already described this exact case. The check sits before the key-presence match, so an unopenable file says so whether or not a key was supplied, rather than first demanding a key that could never work.

CRYPTO-9 and CRYPTO-8

  • CRYPTO-9 — removed a comment explaining why a try_into was infallible; there is no try_into, and the stale reference removed the one hint that a length check ever guarded this. Replaced the prose-only pairing invariant with a debug_assert: read_page_unit_into fills only stride bytes of an 8232-byte stack buffer, so a cipher installed while the stride was still 8192 reads the last 40 bytes as zero-init rather than the real tag and nonce.
  • CRYPTO-8open_body returns the Zeroizing buffer instead of .to_vec()-ing out of it, which defeated its own doc's guarantee one call later. The review then found the seal side does the same thing more often: body_plaintext built root pointers, next_handle and the full named_roots table in a plain Vec dropped as a temporary on every superblock write. Fixed too — the claim now holds in both directions.

Review findings folded in

  • My new test forged "every slot" with a stride-unaware helper. On an encrypted file slots sit 8232 apart, so it forged only slot 0 (offset 0 either way) and silently tore slot 1 — the write landing 40 bytes early, inside the previous unit. It passed only because a torn sibling can never win selection, i.e. it was testing torn-slot fallback. Added a stride-aware helper and switched both this test and the pre-existing hostile-m_cost test, which carried the same bug and the same untrue comment.
  • My own comments invented a historical bug. They claimed the two unwrap copies "had already diverged in how they mapped kdf ids". They had not — that is the wrap side. Corrected, since a comment fabricating history is the same defect class this issue exists to fix.

Verification

718 tests pass, clippy and fmt clean. Both new tests verified non-vacuous by reverting their guards.

…gorithm byte

Closes #117; also closes #103's SUPERBLOCK-RECOVERY-3, which is the same
defect as CRYPTO-4 filed twice.

CRYPTO-7 / CRYPTO-6. The slot wrap and the slot unwrap-trial each existed
twice, in the crate's highest-consequence code. `unlock`'s doc asserted its
copy was "byte-identical" to `unwrap_first_matching_slot`; on the unwrap side
that happened to be true (the copies differed only in how they spelled the
kdf-id match, and the discriminants are 1 and 2). On the WRAP side it was
already false: `wrap_into` wrote the zero argon2 params the on-disk format
documents for HKDF slots, while `build_create_cipher` wrote the OWASP defaults
(19456/2/1) into a slot whose kdf_id said HKDF. So a database created with
Key::Raw disagreed byte-for-byte with a second raw credential added later
through add_key, and neither matched the format doc — the exact drift a
comment claiming equivalence cannot prevent and a shared function can.

`open_existing` now calls `header.unlock(k)`; `build_create_cipher` builds an
empty header and calls `wrap_into`, which gained an `argon2_override`
parameter for the create path's `Options::argon2_params`. Existing databases
are unaffected: `unlock` rebuilds the AAD from the STORED slot bytes and
derives the KEK from the STORED params, and HKDF never reads those fields, so
a legacy slot carrying 19456/2/1 still unwraps. The adversarial review
confirmed that end to end against a file created by the old code.

CRYPTO-4 / SUPERBLOCK-RECOVERY-3. The algorithm byte was written at create and
never read back — `deserialize` gates only on zero ("plaintext"), so any
nonzero value was accepted and every page fed to PageCipher, which is
hardcoded to XChaCha20-Poly1305. The DEK unwrap is a separate primitive that
does not depend on the page algorithm, so such a file OPENED and then failed
later as DecryptionFailed, which is_fatal() and poisons: "this build cannot
read that algorithm" reported to the user as unrecoverable data corruption.
Rejected now with EncryptionNotSupported, whose doc already described this
case. The check sits before the key-presence match, so an unopenable file says
so whether or not a key was supplied, rather than first demanding a key that
could never work.

CRYPTO-9. Removed a comment explaining why a `try_into` was infallible; there
is no try_into, and the stale reference removed the one hint a reader had that
a length check ever guarded this. Replaced the prose-only pairing invariant
with a debug_assert: read_page_unit_into fills only `stride` bytes of an
8232-byte stack buffer, so a cipher installed while the stride was still 8192
would read the last 40 bytes as zero-init rather than the real tag and nonce,
and report a configuration mistake as ciphertext corruption.

CRYPTO-8. `open_body` returns the Zeroizing buffer instead of `.to_vec()`-ing
out of it — the old code allocated a plain copy and let the Zeroizing original
be wiped, defeating the guarantee its own doc claims one call later. The
review then pointed out the seal side does the same thing more often:
`body_plaintext` builds root pointers, next_handle and the full named_roots
table in a plain Vec dropped as a temporary on EVERY superblock write, so
every commit of an encrypted DB left a copy in freed un-wiped heap. Fixed too;
the claim now holds in both directions.

Review findings folded in:

  * The new algorithm test forged "every slot" with a helper that seeks at
    page_id * PAGE_SIZE. On an encrypted file slots sit 8232 apart, so it
    forged only slot 0 (offset 0 either way) and silently TORE slot 1 — the
    write landing 40 bytes early, inside the previous unit. It passed only
    because a torn sibling can never win selection, i.e. it was testing
    torn-slot fallback. Added a stride-aware helper and switched both this
    test and the pre-existing hostile-m_cost test, which carried the same bug
    and the same untrue comment.
  * My own comments claimed the two unwrap copies "had already diverged in how
    they mapped kdf ids". They had not — that is the wrap side. Corrected,
    since a comment inventing a historical bug is the same defect class this
    issue exists to fix.
@Xof
Xof changed the base branch from fix/108-txn-counter-write-side to main August 5, 2026 03:41
@Xof
Xof merged commit 5846c61 into main Aug 5, 2026
9 checks passed
@Xof
Xof deleted the fix/117-crypto-cleanups branch August 5, 2026 03:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Cleanups in the on-disk encryption

1 participant